[Search v2] Add displaying advanced filter values and type/status #46022
Conversation
Kicu
force-pushed
the
kicu/45026-advanced-filter-values
branch
from
346b968
to
b8c4643
Compare
|
@luacmartins this is basically done, but I need code from date filter: #45756 I'm also moving the conversation about form/input flows here.
I've handled this for I don't think its good practice to have some of them be menu items which navigate, while some be We already do it like that for example here: ReportSettings has a button with redirects: https://github.com/Expensify/App/blob/main/src/pages/settings/Report/ReportSettingsPage.tsx#L63 |
Kicu
force-pushed
the
kicu/45026-advanced-filter-values
branch
from
b8c4643
to
22481db
Compare
Kicu
force-pushed
the
kicu/45026-advanced-filter-values
branch
from
22481db
to
9aa2ffb
Compare
|
@rayane-djouah Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Even though the whole filters are kinda dependent on #45617 this PR works as-is and can be reviewed |
luacmartins
changed the title
src/CONST.ts
Outdated
| TYPE: { | ||
| EXPENSES: 'expenses', | ||
| }, |
There was a problem hiding this comment.
Can we move this to DATA_TYPES and use singular expense please?
There was a problem hiding this comment.
Ahh sorry unfortunately that won't work. DATA_TYPES are connected via typedefinition with the mechanism of rendering data in lists in SearchUtils:
https://github.com/Expensify/App/blob/main/src/libs/SearchUtils.ts#L218
If I add expense to this const, then I would have to implement all these functions for displaying an expense. So let's keep them as TYPE I think?
Btw this beautifully shows us that we have too many "types" that mean different things 😉
|
@luacmartins pushed some of your changes. How about we wait with the CONST changes for Adam's PR to be merged, I will make sure that whatever was done there will be the final version. |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-07-29.at.4.34.17.PM.movAndroid: mWeb ChromeScreen.Recording.2024-07-29.at.4.29.59.PM.moviOS: NativeSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-07-29.at.16.32.41.mp4iOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-07-29.at.16.28.45.mp4MacOS: Chrome / SafariScreen.Recording.2024-07-29.at.4.24.51.PM.movMacOS: DesktopScreen.Recording.2024-07-29.at.4.28.07.PM.mov |
| // TODO once we have values from query, this value should be `filters[fieldName].value` | ||
| return fieldName; | ||
| // the values of dateBefore+dateAfter map to just a single 'date' field on advanced filters | ||
| type AvailableFilters = ValueOf<typeof INPUT_IDS> | 'date'; |
There was a problem hiding this comment.
I think it's better if we define the available filter names as constants in the CONST file, like this:
in CONST.SEARCH add:
ADVANCED_SEARCH_FILTERS: {
FIELDS_NAMES: {
DATE: 'date',
...
},
},
Then, use ValueOf<typeof CONST.ADVANCED_SEARCH_FILTERS.FIELDS_NAMES> and replace the hardcoded values with the constants.
There was a problem hiding this comment.
So you suggest I have both the INPUT_IDS and also ADVANCED_SEARCH_FILTERS and use them here?
Btw this is most likely the only case where there are 2 inputs that map to 1 filter, because the actual filter will be date > xxxx and date < yyyy.
I think all the other filters will map 1-to-1 filter name to input.
There was a problem hiding this comment.
Maybe we can define it as follows:
const FIELDS_NAMES = Object.entries(INPUT_IDS).reduce<Record<string, string>>((fields, [key, value]) => {
if (key === INPUT_IDS.DATE_AFTER || key === INPUT_IDS.DATE_BEFORE) {
fields.DATE = 'date';
} else {
fields[key] = value;
}
return fields;
}, {});
type AvailableFilters = ValueOf<typeof FIELDS_NAMES>;My concern here is to avoid using hardcoded values when referencing getFilterDisplayTitle, we should be using FIELDS_NAMES const instead.
There was a problem hiding this comment.
@rayane-djouah after we merged the other PR with new query/syntax - CONSTs were modified, and now I can use better more fitting consts specific to query here.
So I didn't do it your way, but there will be no hardcoded values used here anymore
|
Nice, now that the syntax PR is merged, let's update this one to work with that! |
| // TODO once we have values from query, this value should be `filters[fieldName].value` | ||
| return fieldName; | ||
| // the values of dateBefore+dateAfter map to just a single 'date' field on advanced filters | ||
| type AvailableFilters = ValueOf<typeof INPUT_IDS> | 'date'; |
There was a problem hiding this comment.
Maybe we can define it as follows:
const FIELDS_NAMES = Object.entries(INPUT_IDS).reduce<Record<string, string>>((fields, [key, value]) => {
if (key === INPUT_IDS.DATE_AFTER || key === INPUT_IDS.DATE_BEFORE) {
fields.DATE = 'date';
} else {
fields[key] = value;
}
return fields;
}, {});
type AvailableFilters = ValueOf<typeof FIELDS_NAMES>;My concern here is to avoid using hardcoded values when referencing getFilterDisplayTitle, we should be using FIELDS_NAMES const instead.
| const advancedFilters = useMemo( | ||
| () => [ | ||
| { | ||
| title: getFilterDisplayTitle({}, 'title'), | ||
| title: getFilterDisplayTitle(searchAdvancedFilters, 'type', translate), |
There was a problem hiding this comment.
For example, here we can use FIELDS_NAMES.TYPE instead of 'type'
|
Bug 1: "Expense" option is focused initially and has a green check icon even if it's not selected Screen.Recording.2024-07-25.at.8.10.00.PM.movBug 2: All status options have a green check icon (I believe only the selected one should have this icon) Screen.Recording.2024-07-25.at.8.10.34.PM.movBug 3: Status is not updated in Onyx and in the advanced filters page Screen.Recording.2024-07-25.at.8.10.43.PM.mov |
|
There are conflicts |
I think this one is expected. Expense is the default type and we should always have a selected type (even if not in the query itself) |
Hm, in this case it should be also selected by default in the menu item in the advanced filters page: 
|
|
Yea, I agree with that. Same goes for status, if nothing is selected |
|
I fixed some of the comments but not all of them, will continue on Monday, please don't re-review yet. |
|
@rayane-djouah ready for re-review. However tested only on web now, since native builds take a lot of time. Will test on native later and update this comment. I'm not able to reproduce any of the 3 bugs you mentioned - I hope they are all fixed. What should and should NOT work:
CC @luacmartins I will clean and fix whatever else in separate PRs :) |
| // TODO once we have values from query, this value should be `filters[fieldName].value` | ||
| return fieldName; | ||
| function getFilterDisplayTitle(filters: Partial<SearchAdvancedFiltersForm>, fieldName: AdvancedFiltersKeys, translate: LocaleContextProps['translate']) { | ||
| if (fieldName === 'date') { |
There was a problem hiding this comment.
| if (fieldName === 'date') { | |
| if (fieldName === CONST.SEARCH.SYNTAX_FILTER_KEYS.DATE) { |
| const dateBefore = filterValues[INPUT_IDS.DATE_BEFORE]; | ||
| const dateAfter = filterValues[INPUT_IDS.DATE_AFTER]; | ||
|
|
||
| let dateFilter = ''; | ||
| if (dateBefore) { | ||
| dateFilter += `${CONST.SEARCH.SYNTAX_FILTER_KEYS.DATE}<${dateBefore}`; | ||
| } | ||
| if (dateBefore && dateAfter) { | ||
| dateFilter += ' '; | ||
| } | ||
| if (dateAfter) { | ||
| dateFilter += `${CONST.SEARCH.SYNTAX_FILTER_KEYS.DATE}>${dateAfter}`; | ||
| } |
There was a problem hiding this comment.
NAB: We can simplify buildQueryStringFromFilters function by extracting the date handling logic to a new buildDateFilterQuery function
|
@Kicu - Could you please update the test steps in the author checklist to make it clear for the QA team. I think this is not relevant anymore:
|
| buttonText={translate('search.viewResults')} | ||
| containerStyles={[styles.m4]} | ||
| onSubmit={onFormSubmit} | ||
| /> |
There was a problem hiding this comment.
I think that the "View results" button should not be disabled offline. based on OfflineUX_Patterns_Flowchart, No offline pattern is needed here.
| /> | |
| enabledWhenOffline | |
| /> |
There was a problem hiding this comment.
I think we can keep it as is for now so that we don't block other issues, but I think we should disable it offline because the Search API command only works while the user is online.
|
@rayane-djouah code comments fixed. About Bug 4 the filtering does not happen because I don't yet send values for the filters in the backend call to |
|
I think it's fine to handle that in a follow up. |
| ); | ||
|
|
||
| const onFormSubmit = () => { |
There was a problem hiding this comment.
NAB:
| const onFormSubmit = () => { | |
| const openResults = () => { |
From the Reviewer Checklist:
I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
|
cc @Expensify/design to review the new pages design Screenshots/VideosAndroid: NativeScreen.Recording.2024-07-29.at.4.34.17.PM.movAndroid: mWeb ChromeScreen.Recording.2024-07-29.at.4.29.59.PM.moviOS: NativeSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-07-29.at.16.32.41.mp4iOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-07-29.at.16.28.45.mp4MacOS: Chrome / SafariScreen.Recording.2024-07-29.at.4.24.51.PM.movMacOS: DesktopScreen.Recording.2024-07-29.at.4.28.07.PM.mov |
| TYPE: { | ||
| EXPENSE: 'expense', | ||
| }, |
There was a problem hiding this comment.
NAB we should eventually move this to CONST.SEARCH.DATA_TYPES.EXPENSE and get rid of transactions and reports. We'll do so as part of v2.2.
| * Given object with chosen search filters builds correct query string from them | ||
| */ | ||
| function buildQueryStringFromFilters(filterValues: Partial<SearchAdvancedFiltersForm>) { | ||
| // TODO add handling of multiple values picked |
There was a problem hiding this comment.
| // TODO add handling of multiple values picked |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Cherry-picked to staging by https://github.com/roryabraham in version: 9.0.14-1 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.0.14-6 🚀
|
Details
This is another step in partial implementation of Advanced Search filters.
Waiting for merging of: #45756 and #45617This PR does:
typefilter (we support only 1 type for now)statusfilterWhat is not done:
queryfor default values of formNotes:
SearchFiltersStatusPageandSearchFiltersTypePageare extremely similar, but before extracting common code I want to see some more pagesValuePickerbecause it renders an extra screen and requires extra click, whereas we need the list of options directly on PageFixed Issues
$ #45026
$ #46029
$ #46028
PROPOSAL:
Tests
/search/filtersOffline tests
QA Steps
/search/filtersPR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
rec-andr-filtervals.mp4
Android: mWeb Chrome
// had problems running on android
iOS: Native
rec-ios-filtervals.mp4
iOS: mWeb Safari
MacOS: Chrome / Safari
rec-web-filtervals.mp4
MacOS: Desktop